Repository navigation
Add MCP tools for snapshot export, import, update and device target - #8497
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8497 +/- ##
==========================================
+ Coverage 77.07% 77.13% +0.05%
==========================================
Files 466 466
Lines 24943 25004 +61
Branches 6643 6664 +21
==========================================
+ Hits 19226 19287 +61
Misses 5717 5717
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
andypalmi
left a comment
There was a problem hiding this comment.
All four tools line up with the routes and controllers, and the tricky bits are handled well: the partial update with the empty-name guard, the settings.env normalization that dodges the unguarded Object.keys in uploadSnapshot, and making the device target snapshot required so the route (which never replies when it is missing) can't hang. Tests cover them nicely.
One optional cleanup, same theme as the pipeline stage tools: platform_export_snapshot and platform_import_snapshot carry near-identical components schemas. The differences are just the direction wording and the "exposes hidden values" caution, and that caution already lives in the export tool's description, so repeating it in the arg is the kind of duplication worth dropping. Could this be a single shared schema in schemas.js (next to snapshotId/hostedInstanceId), spread into both?
// schemas.js
const snapshotComponents = z.object({
flows: z.boolean().optional().describe('Include flows (default true). Excluding flows also excludes credentials'),
credentials: z.boolean().optional().describe('Include the flow credentials (default true)'),
envVars: z.union([z.literal('all'), z.literal('keys'), z.literal(false)]).optional().describe('Environment variables: "all" keeps keys and values (default), "keys" keeps only the names, false removes them')
}).optional()And a smaller one: a couple of descriptions restate mechanics that already live in the args, for example the non-empty name and empty-string-to-clear notes on platform_update_snapshot. Could those stay only in the arg .describe() that owns them, keeping the description tool-level? The owner-resolution, partial-update, and immediate-deploy caution are genuinely tool-level and read well where they are.
None of this is blocking, happy for it to be a follow-up if you would rather keep this PR focused.
…s, enforce upfront validation for encrypted data, and handle excluded components properly. Add tests for various encrypted scenarios.
|
Pushed a follow-up to the import tool after a closer read of the controller. Hidden env vars are also exported encrypted (the env entry gets a
Since both live in the route/controller, the tool handles them client-side for now:
Tests cover the strip, both rejection paths, and the two cases that should still go through. Guarding the decrypt loop in |
Mark platform_set_instance_device_target destructive: it overwrites the target on every device assigned to the instance, so it belongs behind destructive tool access rather than plain write. Guard platform_update_snapshot: tool input is not validated platform-side, so a blank name reached the controller and surfaced as a 500, and an update with no fields got a 200 with the snapshot unchanged. Reduce env vars to their names up front on a keys-only import. The route discards the values anyway, but decrypts the hidden ones first, which forced a credentialSecret the caller does not need. Share one components schema between export and import, use the toolError helper for the tool's own 400, and keep arg-level mechanics in the arg descriptions rather than repeating them in the tool description.
Accept an export verbatim: the snapshot argument is loose now, so the six fields an export carries on top of name/description/flows/settings no longer fail validation. Only the four the route reads are forwarded. Reject unencrypted flow credentials, which the route stores in the clear (#8569), and stop asking for a credential secret when components.flows is false, where the secret is never used (#8570). Give settings.env a declared shape so a null value cannot reach the route, which reads every value's properties unguarded (#8568). Document the FF_ env var stripping, and the credentials block every export carries whatever the snapshot holds (#8571), on both tool descriptions.
|
Raised the endpoint-side findings from testing these tools as separate issues, since the tools here only work around them rather than fix them:
Also #8572 for the gateway dropping the status, |
The line claiming an export always carries a credentials block was only half right: with components credentials:false or flows:false the export carries an empty object and the import needs no secret at all. Say which case is which. A blank credentialSecret now fails in the tool rather than costing a round trip to the route, which reads it as absent and answers 400. Matches what the update tool already does for a blank name. Note that envVars:"keys" drops the hidden flag, so a secret variable comes back as an ordinary empty one. That lives on the shared components schema, so it covers the import tool too. Also carry over the payload-size caution from platform_get_snapshot_full, since an export is always a superset of it.
|
Two more from testing
The tool-side findings from the same pass are fixed in 4a587ea:
Everything else in the export tool checked out: the unconditional |
The name column is 255 wide, so a longer value comes back as a 500 carrying the raw database error (#8576). The tool already shields a blank name for the same reason, this is the other end of the same check. Only bites on postgres: sqlite ignores the declared width, so the guard is in the handler as well as the schema rather than relying on a test that would pass on the default dev database either way.
|
One new thing, fixed in 2666b0a: a name over 255 characters was going through to a 500 carrying the raw database error, since the column is Only bites on postgres, sqlite ignores the declared width, so the guard sits in the handler as well as the schema rather than leaning on a test that would pass on the default dev database regardless. Also corrected #8575 while I was there. I'd guessed the gateway might be skipping other schema keywords too, but |
…t radius "This tool can only set a target, not clear one" read as a limitation of the tool, so an agent would go looking for another way. There isn't one: the route ignores a null target and answers 200 without changing anything. The only way to remove a target is to delete the snapshot it points at, which clears it from the instance and from every assigned device. The description also tells the caller to confirm before deploying to every assigned device, without giving them anything to confirm against. Point at platform_list_remote_instances scoped by hostedInstanceId, which answers exactly which devices a call will hit.
|
Tested against a throwaway application and instance with two devices assigned to it, so the deploy path ran for real. Everything in the description checks out:
Two description fixes in 9bbd6e0:
Didn't raise an endpoint issue for the missing clear, it looks deliberate rather than broken. Happy to raise it as a question if anyone disagrees. Also added a third data point to #8575: |
andypalmi
left a comment
There was a problem hiding this comment.
The latest commits cover everything from the last pass: shared components schema, the blank-secret and name-length guards on both ends, and the device-target description now spelling out what it can and cannot undo. Splitting the endpoint and gateway findings into their own issues is the right call, since the tools only work around them here.
On #8574, that role ordering is pre-existing rather than something this PR introduces, so it should not hold this up. Fine to leave to triage on that issue and change the behaviour later if we decide to.
Approving.
Closes #7688
Adds four snapshot write tools:
platform_update_snapshotwrapsPUT /api/v1/snapshots/:id. The controller does proper partial updates, so the tool only sends the fields it was given. A blank name is not cleanly rejected: the controller throws a sequelizeValidationErrorand nothing maps that to a status code, so the route answers 500. The tool catches it first (whitespace only counts as blank, since the controller trims) and returns a 400. An update with no fields at all gets a 200 back with the snapshot unchanged, which reads as a successful edit that never happened, so the tool rejects that too.platform_export_snapshotwrapsPOST /api/v1/snapshots/:id/export.credentialSecretis unconditionally required by the route (400 without it), and the description warns that the default export includes hidden env var values, plus a reminder that the same secret is needed at import time.platform_import_snapshotwrapsPOST /api/v1/snapshots/import. One real quirk surfaced while reading the controller:uploadSnapshotcallsObject.keys(snapshot.settings.env)unguarded, so a snapshot withoutsettings.env500s. The handler normalises an omitted env to{}so agents don't hit that. Same story for hidden env values, which get decrypted before the component filtering runs, so a keys-only or env-excluded import would 500 without a secret it does not actually need; the tool reduces env up front in both cases and the result is identical to what the route produces on its happy path. One rough edge left: the export response carries six extra fields (id,createdAt,updatedAt,ownerType,user,exportedBy) that this tool'ssnapshotargument does not accept, so pasting an export straight back in fails validation withunrecognized key. The description spells out which four fields to copy, but it might be worth letting the schema ignore the extras so the obvious export then import flow just works.platform_set_instance_device_targetwrapsPOST /api/v1/projects/:id/devices/settings. The description carries a caution that setting the target deploys immediately to every assigned device, and notes the route can only set a target, not clear one. The tool makessnapshotIdrequired because the route's onlyreply.sendsits inside theif (request.body.targetSnapshot)block: omit it and you get a 200 with an empty body and nothing changed, a silent no-op rather than an error. It is also annotateddestructiveHint: true, since it overwrites what every assigned device is running rather than adding to it, which puts it behind destructive tool access instead of plain write.Descriptions were written from the actual route/controller behavior rather than assumptions, including the owner resolution (instance or device) happening from the snapshot itself. The behaviors above were checked against a running platform, calling each route directly and invoking the matching tool with the same arguments.